-
Notifications
You must be signed in to change notification settings - Fork 133
[Documentation] Nvidia style guide edits to pipeline visualization #1328
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: meg miranda <mmiranda@nvidia.com>
Greptile SummaryThis PR applies NVIDIA style guide edits to the pipeline visualization documentation. The changes improve the document structure and readability by:
The documentation restructure is well-executed and addresses previous feedback. However, three minor grammatical issues remain:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Dev as Developer
participant PR as Pull Request
participant Style as NVIDIA Style Guide
participant Review as Review Process
Dev->>Style: Reference style guide rules
Dev->>PR: Apply edits to README.md
Note over PR: Convert numbered lists to bullets<br/>Reorganize sections<br/>Improve heading hierarchy<br/>Standardize formatting
PR->>Review: Submit for review
Review->>PR: Check against previous feedback
Note over Review: Addresses prior comments:<br/>- Subject-verb agreement<br/>- List numbering<br/>- Comma placement
Review->>PR: Identify new issues
Note over Review: Found 3 minor issues:<br/>- Line 179: parameter values<br/>- Line 171: run-on sentence<br/>- Line 205: verb agreement
Review->>Dev: Approve with minor suggestions
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 file reviewed, 1 comment
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughREADME for Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (5)📓 Common learnings📚 Learning: 2025-11-17T21:37:54.245ZApplied to files:
📚 Learning: 2025-11-11T15:40:28.150ZApplied to files:
📚 Learning: 2025-12-18T16:19:45.117ZApplied to files:
📚 Learning: 2025-10-22T16:33:55.411ZApplied to files:
🪛 LanguageToolapplications/pipeline_visualization/README.md[style] ~147-~147: The verb “get” can be informal. Consider replacing it with a form of “to be”. (GET_USED_ARE_USED) [style] ~205-~205: This phrase is redundant. Consider writing “evolve”. (EVOLVE_OVER_TIME) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (1)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @applications/pipeline_visualization/README.md:
- Line 190: Update the broken link fragment in the README by changing the
reference from #visualizer-python-dependencies to the actual anchor
#visualizer-python-prerequisites so the phrase referring to the Visualizer
Python dependencies points to the "Visualizer Python Prerequisites" section;
locate the link text in the same paragraph referencing the Holohub container and
replace the fragment accordingly.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
applications/pipeline_visualization/README.md
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-12-18T16:19:45.117Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-12-18T16:19:45.117Z
Learning: Applies to **/(@(applications|workflows))/**/README.md : Performance insights and any latency optimizations should be documented in the application README
Applied to files:
applications/pipeline_visualization/README.md
📚 Learning: 2025-10-22T16:33:55.411Z
Learnt from: cdinea
Repo: nvidia-holoscan/holohub PR: 1170
File: applications/video_streaming/video_streaming_client/python/streaming_client_demo_replayer.yaml:27-36
Timestamp: 2025-10-22T16:33:55.411Z
Learning: In the video_streaming bidirectional client applications (applications/video_streaming/video_streaming_client), the pipeline has two separate data paths: (1) Outgoing: source → format_converter → streaming_client INPUT (sends to server), and (2) Incoming: streaming_client OUTPUT → holoviz (receives from server). The format_converter prepares data for transmission and does NOT feed directly into holoviz visualization.
Applied to files:
applications/pipeline_visualization/README.md
📚 Learning: 2025-11-17T21:37:54.245Z
Learnt from: bhashemian
Repo: nvidia-holoscan/holohub PR: 1220
File: applications/pipeline_visualization/README.md:135-137
Timestamp: 2025-11-17T21:37:54.245Z
Learning: In the Holohub repository, when both Python and C++ applications are available for the same application, the Holohub CLI defaults to Python if the `--language` argument is not provided. Users can explicitly specify `--language cpp` or `--language python` to select the desired implementation.
Applied to files:
applications/pipeline_visualization/README.md
📚 Learning: 2025-12-18T16:19:45.117Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-12-18T16:19:45.117Z
Learning: Applies to **/applications/**/CMakeLists.txt : CMakeLists.txt for applications must use 'add_holohub_application()' with optional 'DEPENDS OPERATORS' for dependent operators
Applied to files:
applications/pipeline_visualization/README.md
📚 Learning: 2025-12-18T16:19:45.117Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-12-18T16:19:45.117Z
Learning: Applies to **/*.{py,cpp,hpp} : Code must adhere to Holoscan SDK coding standards including style compliance, descriptive naming, and minimal abbreviations
Applied to files:
applications/pipeline_visualization/README.md
📚 Learning: 2025-11-11T15:40:28.150Z
Learnt from: AndreasHeumann
Repo: nvidia-holoscan/holohub PR: 1220
File: applications/pipeline_visualization/cpp/create_tensor.hpp:21-22
Timestamp: 2025-11-11T15:40:28.150Z
Learning: In the pipeline_visualization application, FlatBuffers generated headers like tensor_generated.h can be included using the simplified path `#include <flatbuffers/tensor_generated.h>` because the CMake target pipeline_visualization_flatbuffers_schemas generates them and pipeline_visualization_flatbuffers_target exposes the correct include directories, which are inherited by targets that link against it.
Applied to files:
applications/pipeline_visualization/README.md
🪛 markdownlint-cli2 (0.18.1)
applications/pipeline_visualization/README.md
190-190: Link fragments should be valid
(MD051, link-fragments)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> Signed-off-by: megnvidia <mmiranda@nvidia.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Greptile Summary
This PR applies NVIDIA style guide edits to the pipeline visualization README, improving documentation formatting and readability. The changes include converting numbered lists to use asterisks for consistency, restructuring command examples with proper indentation, improving punctuation in step-by-step instructions, and replacing phrases like "via" with "using" for clarity.
Key improvements:
- Converted main component list from numbered format to bullet points for consistency (lines 11-13)
- Restructured command-line examples with better organization and indentation (lines 44-55)
- Added periods to all step instructions for consistency (lines 78-81)
- Changed "simple" to "basic" for more professional tone (line 141)
- Improved heading structure and cross-references between sections
Issues found:
- Broken internal link on line 190 that points to non-existent anchor
- Minor grammar issue with unnecessary comma on line 282
Confidence Score: 4/5
- This PR is safe to merge after fixing the broken internal link
- The changes are documentation-only style guide improvements that enhance readability and consistency. One critical issue (broken internal link) must be fixed as it will result in a non-functional link, and one minor style improvement would be nice to have
- The README.md file needs attention for the broken link on line 190 before merging
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| applications/pipeline_visualization/README.md | 4/5 | Documentation style guide improvements with mostly good formatting changes, but has a broken internal link and a minor grammar issue |
Sequence Diagram
sequenceDiagram
participant Author as PR Author
participant README as README.md
participant StyleGuide as NVIDIA Style Guide
participant Readers as Documentation Readers
Author->>StyleGuide: Review style guidelines
StyleGuide-->>Author: Formatting rules (bullets, indentation, punctuation)
Author->>README: Convert numbered list to bullets (lines 11-13)
Author->>README: Restructure command examples with indentation (lines 44-55)
Author->>README: Add periods to step instructions (lines 78-81)
Author->>README: Replace "via" with "using" (line 94)
Author->>README: Change "simple" to "basic" (line 141)
Author->>README: Update cross-reference links (line 160, 190)
Author->>README: Improve punctuation consistency throughout
Note over README: Internal link issue:<br/>Line 190 points to wrong anchor
README-->>Readers: Improved formatting and clarity
Readers->>README: Navigate using internal links
Note over Readers,README: Broken link on line 190<br/>needs to be fixed
AndreasHeumann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes, they look good. Please take care of the AI code review comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Greptile Summary
This PR applies NVIDIA style guide edits to the Pipeline Visualization documentation, improving consistency and readability throughout the README.
Key Changes:
- Changed numbered lists to bullet points for component descriptions
- Improved code block formatting with proper indentation for nested steps
- Standardized punctuation (added periods to list items, removed exclamation marks)
- Replaced informal phrases: "via" → "using", "e.g." → "for example"
- Removed bold formatting from non-heading labels like "Command-line Options" and "Example with custom settings"
- Changed "simple" to "basic" for pipeline description
- Reorganized Python Visualizers section: moved shared information before subsection descriptions for better flow
- Improved heading consistency: "1. Static Visualizer" → "Static Visualizer"
- Enhanced troubleshooting steps with proper numbered list formatting and code formatting
All changes are purely stylistic and documentation-focused with no functional code modifications.
Confidence Score: 5/5
- This PR is completely safe to merge with zero risk
- Score reflects that this is purely a documentation formatting update with no code changes, no functional modifications, and all edits improve readability and consistency according to NVIDIA style guidelines
- No files require special attention
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| applications/pipeline_visualization/README.md | 5/5 | Style guide edits applied throughout: changed numbered lists to bullets, improved code block formatting, standardized punctuation, replaced informal phrases (e.g., via→using, e.g.→for example), and reorganized visualizer documentation for clarity |
Sequence Diagram
sequenceDiagram
participant Author as PR Author
participant README as README.md
participant StyleGuide as NVIDIA Style Guide
Author->>StyleGuide: Review style guidelines
Author->>README: Apply formatting changes
Note over README: Bullet points instead of numbers
Note over README: Improved code block formatting
Note over README: Standardized punctuation
Note over README: Replaced informal phrases
Author->>README: Reorganize visualizer section
Note over README: Move common info before subsections
Note over README: Update headings and references
Author->>README: Commit changes
README-->>Author: Documentation updated
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> Signed-off-by: Bruce Hashemian <3968947+bhashemian@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
5 files reviewed, 5 comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
6 files reviewed, 6 comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 file reviewed, 4 comments
Signed-off-by: meg miranda <mmiranda@nvidia.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 file reviewed, 1 comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@applications/pipeline_visualization/README.md`:
- Line 208: Replace the affected README sentence "FlatBuffers access the data
directly without unpacking or parsing it and allows the the schema to evolve
over time while still maintaining forwards and backwards." by removing the
duplicate "the the", change "evolve over time" to "evolve", and add
"compatibility" to complete the phrase; e.g., reword to "FlatBuffers accesses
data directly without unpacking or parsing and allows the schema to evolve while
maintaining forward and backward compatibility."
🧹 Nitpick comments (3)
applications/pipeline_visualization/README.md (3)
149-149: Consider more formal phrasing.The phrase "get installed" could be replaced with "are installed" for a more formal tone.
✍️ Suggested revision
-The packages that get installed are: +The packages that are installed are:Alternatively, simplify to:
-The packages that get installed are: +The required packages are:
173-173: Simplify the sentence structure.The current phrasing "One is...and one is...and they both" creates a repetitive flow. Consider restructuring for improved readability.
✍️ Suggested revision
-There are two Python visualizers. One is [static](`#static-visualizer-visualizer_staticpy`) and one is [dynamic](`#dynamic-visualizer-visualizer_dynamicpy`) and they both display: +There are two Python visualizers: [static](`#static-visualizer-visualizer_staticpy`) and [dynamic](`#dynamic-visualizer-visualizer_dynamicpy`). Both display:
262-262: Add missing article for grammatical correctness.The phrase "dynamic visualizer" should be preceded by the article "the" for proper grammar.
✍️ Suggested revision
-3. For dynamic visualizer, no changes needed - it will auto-discover. +3. For the dynamic visualizer, no changes needed - it will auto-discover.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
applications/pipeline_visualization/README.md
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-12-18T16:19:45.117Z
Learning: Applies to **/(@(applications|workflows))/**/README.md : Performance insights and any latency optimizations should be documented in the application README
Learnt from: cdinea
Repo: nvidia-holoscan/holohub PR: 1170
File: applications/video_streaming/video_streaming_client/python/streaming_client_demo_replayer.yaml:27-36
Timestamp: 2025-10-22T16:33:55.411Z
Learning: In the video_streaming bidirectional client applications (applications/video_streaming/video_streaming_client), the pipeline has two separate data paths: (1) Outgoing: source → format_converter → streaming_client INPUT (sends to server), and (2) Incoming: streaming_client OUTPUT → holoviz (receives from server). The format_converter prepares data for transmission and does NOT feed directly into holoviz visualization.
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-12-18T16:19:45.117Z
Learning: Applies to **/(@(operators|applications|workflows|gxf_extensions))/**/README.md : Every application and operator must include a comprehensive README.md file with purpose, usage, requirements, examples, and architecture information
📚 Learning: 2025-11-17T21:37:54.245Z
Learnt from: bhashemian
Repo: nvidia-holoscan/holohub PR: 1220
File: applications/pipeline_visualization/README.md:135-137
Timestamp: 2025-11-17T21:37:54.245Z
Learning: In the Holohub repository, when both Python and C++ applications are available for the same application, the Holohub CLI defaults to Python if the `--language` argument is not provided. Users can explicitly specify `--language cpp` or `--language python` to select the desired implementation.
Applied to files:
applications/pipeline_visualization/README.md
📚 Learning: 2025-11-11T15:40:28.150Z
Learnt from: AndreasHeumann
Repo: nvidia-holoscan/holohub PR: 1220
File: applications/pipeline_visualization/cpp/create_tensor.hpp:21-22
Timestamp: 2025-11-11T15:40:28.150Z
Learning: In the pipeline_visualization application, FlatBuffers generated headers like tensor_generated.h can be included using the simplified path `#include <flatbuffers/tensor_generated.h>` because the CMake target pipeline_visualization_flatbuffers_schemas generates them and pipeline_visualization_flatbuffers_target exposes the correct include directories, which are inherited by targets that link against it.
Applied to files:
applications/pipeline_visualization/README.md
📚 Learning: 2025-12-18T16:19:45.117Z
Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-12-18T16:19:45.117Z
Learning: Applies to **/(@(operators|applications|workflows|gxf_extensions|tutorials))/**/README.md : README.md should use HoloHub glossary terms when referring to project-specific locations and concepts
Applied to files:
applications/pipeline_visualization/README.md
📚 Learning: 2025-10-22T16:33:55.411Z
Learnt from: cdinea
Repo: nvidia-holoscan/holohub PR: 1170
File: applications/video_streaming/video_streaming_client/python/streaming_client_demo_replayer.yaml:27-36
Timestamp: 2025-10-22T16:33:55.411Z
Learning: In the video_streaming bidirectional client applications (applications/video_streaming/video_streaming_client), the pipeline has two separate data paths: (1) Outgoing: source → format_converter → streaming_client INPUT (sends to server), and (2) Incoming: streaming_client OUTPUT → holoviz (receives from server). The format_converter prepares data for transmission and does NOT feed directly into holoviz visualization.
Applied to files:
applications/pipeline_visualization/README.md
🪛 LanguageTool
applications/pipeline_visualization/README.md
[style] ~149-~149: The verb “get” can be informal. Consider replacing it with a form of “to be”.
Context: ...requirements.txt ``` The packages that get installed are: - numpy>=1.24.0,<3.0 - Numerica...
(GET_USED_ARE_USED)
[style] ~208-~208: This phrase is redundant. Consider writing “evolve”.
Context: ...parsing it and allows the the schema to evolve over time while still maintaining forwards and ba...
(EVOLVE_OVER_TIME)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🔇 Additional comments (1)
applications/pipeline_visualization/README.md (1)
1-373: Excellent documentation structure and comprehensiveness.The README is well-organized with clear sections covering all essential aspects:
- Quick Start guide with step-by-step instructions
- Architecture diagrams
- Detailed configuration options
- Performance considerations
- Comprehensive troubleshooting guide
The additions align well with documentation best practices and provide valuable guidance for users.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> Signed-off-by: megnvidia <mmiranda@nvidia.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 file reviewed, 1 comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 file reviewed, 1 comment
Signed-off-by: B Hashemian <3968947+bhashemian@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 file reviewed, 3 comments
| - Acquisition timestamp (nanoseconds) | ||
| - Publish timestamp (nanoseconds) | ||
|
|
||
| Use `start_visualizer.sh` to set the required Python path to the [flatbuffers](#flatbuffers-schemas-schemas) definitions and start the visualizer. The script takes one parameter, its values are: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
syntax: Grammatical inconsistency: "takes one parameter, its values are" - if there's one parameter, should be "its possible values are" or "its value can be"
| Use `start_visualizer.sh` to set the required Python path to the [flatbuffers](#flatbuffers-schemas-schemas) definitions and start the visualizer. The script takes one parameter, its values are: | |
| Use `start_visualizer.sh` to set the required Python path to the [flatbuffers](#flatbuffers-schemas-schemas) definitions and start the visualizer. The script takes one parameter, its possible values are: |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| ### Python Visualizers (`visualizer/`) | ||
|
|
||
| Two visualization options are provided: | ||
| There are two Python visualizers. One is [static](#static-visualizer-visualizer_staticpy) and one is [dynamic](#dynamic-visualizer-visualizer_dynamicpy) and they both display: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Run-on sentence with repetitive structure. The three "and" conjunctions create awkward flow.
| There are two Python visualizers. One is [static](#static-visualizer-visualizer_staticpy) and one is [dynamic](#dynamic-visualizer-visualizer_dynamicpy) and they both display: | |
| There are two Python visualizers: a [static](#static-visualizer-visualizer_staticpy) visualizer and a [dynamic](#dynamic-visualizer-visualizer_dynamicpy) visualizer. Both display: |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| - `pandas>=2.3.3,<3.0` - Data manipulation | ||
|
|
||
| ## Data Format | ||
| FlatBuffers access the data directly without unpacking or parsing it and allow the schema to evolve over time while maintaining forward and backward compatibility. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
syntax: Subject-verb agreement issue: "FlatBuffers" as a technology name is singular, but "access" and "allow" are plural verbs
| FlatBuffers access the data directly without unpacking or parsing it and allow the schema to evolve over time while maintaining forward and backward compatibility. | |
| FlatBuffers accesses the data directly without unpacking or parsing it and allows the schema to evolve over time while maintaining forward and backward compatibility. |
nvidia style guide edits to Live Streaming Data Web Dashboard with NATS
I would provide the link to the style guide and cheat sheet for it here, but the URL it leaves are horribly long.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.